Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mksurfdata_esmf: Refactor mksoiltex #2737

Open
wants to merge 3 commits into
base: b4b-dev
Choose a base branch
from

Conversation

samsrabin
Copy link
Collaborator

@samsrabin samsrabin commented Aug 30, 2024

Description of changes

Refactors mksoiltex() subroutine to make it easier to understand and avoid duplicated code.

Specific notes

Contributors other than yourself, if any: None

CTSM Issues Fixed (include github issue #): None

Are answers expected to change (and if so in what way)? No

Any User Interface Changes (namelist or namelist defaults changes)? No

Does this create a need to change or add documentation? Did you do so? No

Testing performed, if any:

  • Builds without error
  • Runs without error
  • Outputs are bit-for-bit identical with previous version (organic_o option 1)
  • Outputs are bit-for-bit identical with previous version (disabled organic_o option 2) Removing that code

Other remaining tasks

  • Rename is_neg_4 to be more meaningful. I think it indicates sand dunes, but is that really true for ALL variables or just PCT_SAND?
  • Can block at TODO: Remove as unnecessary? be removed?
  • Remove organic_o option 2

@samsrabin samsrabin added code health improving internal code structure to make easier to maintain (sustainability) support tools only bfb bit-for-bit labels Aug 30, 2024
@samsrabin samsrabin requested a review from slevis-lmwg August 30, 2024 20:12
@samsrabin samsrabin added the priority: low Background task that doesn't need to be done right away. label Aug 30, 2024
@samsrabin
Copy link
Collaborator Author

Output surface dataset at 1.9x2.5 resolution as of 9a2759f is bit-for-bit identical with version from ctsm5.2.027. However, my implementation of organic_o option 2—which is hard-coded off, as before—produces differences when forced on. I wonder if it would make sense at this point to just remove that code?

@samsrabin samsrabin added the PR status: awaiting review Work on this PR is paused while waiting for review. label Sep 4, 2024
@slevis-lmwg
Copy link
Contributor

@samsrabin I think it's ok to remove "option 2" at this point. There's record of it if we ever decide to resuscitate it.

Also, is this PR affected by the latest updates that occurred here?

@samsrabin
Copy link
Collaborator Author

Thankfully no, this touches a different part of the code.

@samsrabin samsrabin removed the PR status: awaiting review Work on this PR is paused while waiting for review. label Sep 25, 2024
@samsrabin samsrabin added test: mksurfdata Test mksurfdata_esmf before merging and removed support tools only labels Oct 3, 2024
@ekluzek ekluzek added this to the CESM3 Answer changing freeze milestone Dec 5, 2024
@samsrabin samsrabin removed the priority: low Background task that doesn't need to be done right away. label Dec 5, 2024
@samsrabin samsrabin self-assigned this Dec 5, 2024
Copy link
Contributor

@slevis-lmwg slevis-lmwg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samsrabin thanks again for this refactor. I like what you've done and thanks for keeping the comments indicating, for example, what -4 means and so on.

I think I looked this over previously and didn't formally review/approve it at the time. Based on our earlier conversations here, I see that you have a few unchecked checkboxes, but I'm happy to approve preemptively.

end if
if (sand_o(no,1) < 0._r4) then
if (int(sand_o(no,1)) == -4) then
sand_o(no,:) = 99._r4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samsrabin I haven't looked at this in a long while, but judging from sand_o + clay_o = 100 when these two start equal to -4, I will agree with your assessment that the flag -4 indicates sand dunes regardless of which variable we look at.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bfb bit-for-bit code health improving internal code structure to make easier to maintain (sustainability) test: mksurfdata Test mksurfdata_esmf before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants